-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement node Timer api when running in node environment. #4622
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4622 +/- ##
==========================================
+ Coverage 56.17% 56.37% +0.19%
==========================================
Files 194 194
Lines 6544 6551 +7
Branches 3 3
==========================================
+ Hits 3676 3693 +17
+ Misses 2867 2857 -10
Partials 1 1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Only thing I'd change is to not overload the FakteTimers
constructor and instead use an object, but no biggie
refToId: (ref: number) => ref, | ||
}; | ||
|
||
this.fakeTimers = new FakeTimers( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the constructor take an object instead of multiple arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Updated.
const timer2 = env1.global.setInterval(() => {}, 0); | ||
|
||
[timer1, timer2].forEach(timer => { | ||
expect(timer.id).not.toBe(undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not.toBeUndefined()
CI has finished succesfully, not sure why it's still reported as in progress here. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fixes #4559. Not entirely sure if this is the right approach. Let me know if there's a better way!
Some thoughts:
we might want to assert if ref/unref are really functions but I noticed this isn't always done in other places in the codebaseI had to shuffle the FakeTimer constructor args a little bit since some are optional. I considered making timerConfig an optional arg with a default implementation but given that FakeTimer is generic about TimerRef, I couldn't get Flow to typecheck it properly.Any feedback is more then welcome.
Thanks!